Skip to content

feat: multi_scalar_mul blackbox func - #6097

Merged
TomAFrench merged 2 commits into
masterfrom
04-30-feat_multi_scalar_mul_blackbox_func
May 7, 2024
Merged

feat: multi_scalar_mul blackbox func#6097
TomAFrench merged 2 commits into
masterfrom
04-30-feat_multi_scalar_mul_blackbox_func

Conversation

@benesjan

@benesjan benesjan commented Apr 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes noir-lang/noir#4928
Fixes noir-lang/noir#4932

Note: Noticed that we have lookup table for fixed base in BB. Not sure if it's still needed after nuking the fixed based scalar mul.

@benesjan Graphite App

ghost commented Apr 30, 2024 •
edited by benesjan
Loading

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @benesjan and the rest of your teammates on Graphite Graphite

@benesjan benesjan changed the title feat: multi_scalar_mul blackbox func feat: multi_scalar_mul blackbox func Apr 30, 2024
TomAFrench
TomAFrench reviewed Apr 30, 2024
View reviewed changes
Comment thread noir/noir-repo/acvm-repo/brillig/src/black_box.rs Outdated
Comment thread noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs Outdated
@benesjan
benesjan force-pushed the 04-30-feat_multi_scalar_mul_blackbox_func branch 3 times, most recently from 34ecebb to cb0745a Compare May 2, 2024 10:18
TomAFrench
TomAFrench reviewed May 2, 2024
View reviewed changes
Comment thread barretenberg/cpp/src/barretenberg/dsl/acir_format/multi_scalar_mul.cpp Outdated
cycle_scalar_ct scalar(scalar_low_as_field, scalar_high_as_field);

// We multiply input point with the scalar to get the output point of this iteration
auto iteration_output_point = input_point * scalar;

ghost May 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning for an MSM opcode is that we have optimised versions of MSM in barretenberg which avoid doing many inversions when there's lots of terms in the sum. It would be good to use this rather than brute forcing it like this.

Fairly sure it's called batch_mul but can't check rn.

ghost May 2, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok ok, will refactor it. Thanks

ghost May 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aztec-packages/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp

Lines 184 to 192 in 8643922

// compute a multi-scalar-multiplication by creating a precomputed lookup table for each point,
// splitting each scalar multiplier up into a 4-bit sliding window wNAF.
// more efficient than batch_mul if num_points < 4
// only works with Plookup!
template <size_t max_num_bits = 0>
static element wnaf_batch_mul(const std::vector<element>& points, const std::vector<Fr>& scalars);
static element batch_mul(const std::vector<element>& points,
const std::vector<Fr>& scalars,
const size_t max_num_bits = 0);

ghost May 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Here's a link to it.

@benesjan
benesjan force-pushed the 04-30-feat_multi_scalar_mul_blackbox_func branch from cb0745a to 4940947 Compare May 2, 2024 13:19
@github-actions

ghost commented May 2, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

Changes to circuit sizes

Generated at commit: ff9404d41c99274dca079d56150fe41dfe926467, compared to commit: 3e0553456535cd32743f7cf33e51ffd8a36ff75d

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
private_kernel_tail +1 ❌ +0.00% +659 ❌ +0.06%
private_kernel_tail_to_public +1 ❌ +0.00% +659 ❌ +0.04%
Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
private_kernel_tail 135,880 (+1) +0.00% 1,063,378 (+659) +0.06%
private_kernel_tail_to_public 349,078 (+1) +0.00% 1,536,194 (+659) +0.04%

@benesjan
benesjan force-pushed the 04-30-feat_multi_scalar_mul_blackbox_func branch 2 times, most recently from 5fc4212 to a3f7882 Compare May 3, 2024 07:04
benesjan
benesjan commented May 3, 2024
View reviewed changes
Comment thread barretenberg/cpp/src/barretenberg/dsl/acir_format/multi_scalar_mul.cpp
benesjan
benesjan commented May 3, 2024
View reviewed changes
Comment thread noir-projects/aztec-nr/aztec/src/keys/point_to_symmetric_key.nr
benesjan
benesjan commented May 3, 2024
View reviewed changes
Comment thread noir/noir-repo/acvm-repo/acvm/src/pwg/blackbox/fixed_base_scalar_mul.rs
@@ -1,70 +0,0 @@
// TODO(https://github.com/noir-lang/noir/issues/4932): rename this file to something more generic

ghost May 3, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed this as embedded_curve_ops.rs

@benesjan
benesjan force-pushed the 04-30-feat_multi_scalar_mul_blackbox_func branch from 56dfb38 to 46749ac Compare May 3, 2024 08:18
benesjan
benesjan commented May 3, 2024
View reviewed changes
Comment thread noir/noir-repo/noir_stdlib/src/grumpkin_scalar_mul.nr
@@ -1,6 +0,0 @@
use crate::grumpkin_scalar::GrumpkinScalar;

ghost May 3, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to nuke this as it was used only in 1 place in aztec-packages and it's completely unnecessary.

@benesjan
benesjan marked this pull request as ready for review May 3, 2024 08:49
@benesjan
benesjan requested review from TomAFrench and vezenovm May 3, 2024 08:49
TomAFrench
TomAFrench approved these changes May 7, 2024
View reviewed changes
Comment thread noir/noir-repo/docs/docs/noir/standard_library/cryptographic_primitives/embedded_curve_ops.mdx Outdated
Comment thread barretenberg/cpp/src/barretenberg/dsl/acir_format/multi_scalar_mul.cpp
@TomAFrench
TomAFrench enabled auto-merge (squash) May 7, 2024 09:23
@benesjan
benesjan force-pushed the 04-30-feat_multi_scalar_mul_blackbox_func branch from 2ff2a58 to e4f2a8e Compare May 7, 2024 11:24
@benesjan
benesjan force-pushed the 04-30-feat_multi_scalar_mul_blackbox_func branch from e4f2a8e to b23f7fc Compare May 7, 2024 12:20
@benesjan benesjan mentioned this pull request May 7, 2024
Merged
@TomAFrench
TomAFrench disabled auto-merge May 7, 2024 12:42
@TomAFrench
TomAFrench enabled auto-merge (squash) May 7, 2024 12:42
@TomAFrench
TomAFrench merged commit f6b1ba6 into master May 7, 2024
@TomAFrench
TomAFrench deleted the 04-30-feat_multi_scalar_mul_blackbox_func branch May 7, 2024 14:20
This was referenced May 7, 2024
Merged
Merged
Merged
@kevaundray kevaundray mentioned this pull request May 8, 2024
Merged
@kevaundray kevaundray mentioned this pull request May 21, 2024
Closed
@noirwhal noirwhal mentioned this pull request Jun 4, 2024
Merged
@noirwhal noirwhal mentioned this pull request Jun 17, 2024
Merged
@noirwhal noirwhal mentioned this pull request Jul 18, 2024
Merged
@noirwhal noirwhal mentioned this pull request Aug 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TomAFrench TomAFrench TomAFrench approved these changes
@vezenovm vezenovm Awaiting requested review from vezenovm

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rename fixed_base_scalar_mul.rs Replace FixedBaseScalarMul blackbox function with a MultiScalarMul opcode

2 participants

@benesjan @TomAFrench